Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| // Anything else is literal text: emit through the `${` and rescan right | ||
| // after it, so a stray `${` in one value can't swallow a real reference | ||
| // later in the document. | ||
| let reference = body.find('}').and_then(|end| { |
There was a problem hiding this comment.
I think all characters are allowed in a Postgres password, e.g., $, { and }, so this is a valid password which will be expanded to an empty string:
${hello}
Curious if you have any thoughts. Maybe we should only expand settings that are entirely covered by an env var, e.g.:
password = "${PASSWORD}" # setting value starts with `${` and ends with `}`That would require us to perform shellexpand on each value after deserialization (or write a custom serializer).
Just thinking out loud, let me know what you think.
There was a problem hiding this comment.
In the case with ${hello} it would need to be set as the password value and also be set in the environment, so unless it has an environment variable for hello= it will keep it as the original string, ultimately leaving as ${hello}.
I was definitely concerned with using shellexpand, I could imagine a situation where generated passwords or especially some longer tokens could easily contain something where it would match shorter commonly set environment variables, like CC, where a randomly generated value like ....$CC..... would get expanded to the value of CC with something like gcc. But I feel a lot better with the new non-shellexpand approach being that it requires:
- The environment variable must still be set in the process environment
- It must be a sequence of
${followed by a} - The variable name itself can only contain contain characters
[a-zA-Z0-9_]and cannot start with_or a digit (I based it off of POSIX standard, but including lowercase letters)
I don't know how to go about calculating a probability on it myself, but it seems like it would be practically impossible given the confluence of things that would need to happen coupled with password generators usually don't create passwords that include { or } and tokens like JWTs are usually base64 encoded which would exclude that as well.
I really appreciate the discussion with this btw, I think this kind of thing IME is not something you can think too much about for sure!
There was a problem hiding this comment.
I had another thought that might be a better developer experience and would be even more impossible to match on sequences unintentionally. I have used this other project previously that had a similar capability, but it has a much more specific opening sequence since it can handle both environment variables and reading from files: https://www.apollographql.com/docs/graphos/routing/configuration/yaml#variable-expansion. It uses ${} but needs either env. or file. to specify the environment variable name or file name, respectively.
With an opener of ${env. it means that only sequences matching roughly ${env.([a-zA-Z0-9_])+} would even perform an environment variable lookup. I'm reasonably confident the previous approach would be practically impossible, but this would be without any doubt impossible to do unintentionally. And IMO is a better developer/user experience since it is very obvious reading the configuration file what is happening since it says "env" with each variable name.
Thoughts on this approach? It is trivial to make this change since it is just adjusting the opener.
Configuration files can now reference the process environment, so secrets
and per-environment values no longer have to be baked into the files on
disk:
```toml
[admin]
password = "${PGDOG_ADMIN_PASSWORD}"
[general]
shutdown_timeout = ${PGDOG_SHUTDOWN_TIMEOUT:-60000}
```
`$VAR` and `${VAR}` are substituted from the environment, `${VAR:-value}`
supplies a fallback, and `$$` is a literal `$`.
Lookups are lenient: a reference to a variable that isn't set is left in
the document verbatim rather than failing the load. `users.toml` is the
file most likely to contain a stray `$` — a password like `sup$rsecret`
keeps working instead of turning into a startup failure or, worse, a
silently truncated credential. The one behaviour change to be aware of is
that a literal `$$` in an existing value now collapses to a single `$`;
that is unavoidable once any escape exists.
Expansion runs on the document source before it is parsed, so a variable
is interpolated as TOML rather than as a string. `${PASSWORD}` in value
position still needs its surrounding quotes, and a value containing `"`
or a newline will change how the rest of the document parses. This is
what allows bare `shutdown_timeout = ${VAR}` to work, and it is
documented on `expand`.
Implementation notes:
- New `pgdog-config::expand` module. `expand()` is infallible and returns
`Cow::Borrowed` when there is nothing to substitute, so the common case
costs no allocation.
- `FromToml::from_toml` replaces bare `toml::from_str` at the three sites
that parse config text read from disk: both branches of
`ConfigAndUsers::load` and `bootstrap_logger`. Every other
`toml::from_str` in the tree parses a test literal, where expansion is
unwanted, and is untouched.
- The trait carries a blanket impl over `DeserializeOwned`, so no
per-type boilerplate is needed. `from_toml`, not `from_str`, to avoid
colliding with the crate's many `std::str::FromStr` impls.
- `Error::config` now receives the expanded text, so the line numbers it
reports stay correct when a variable's value contains a newline.
- `ConfigAndUsers` keeps `config_text`/`users_text` as the raw,
unexpanded source. Resolved secrets must not be written back to disk
when the config is reloaded or backed up.
Adds a dependency on `shellexpand`.
The expand environment variable feature for configuration files previously used shellexpand to expand references before being parsed as toml. shellexpand supports expanding references that include just a $ so it is being replaced here with a simple scanner over the toml input string that only allows bracketed variable references. The replacement expand function works with the former fallback syntax. Another big bonus for this change is that requiring brackets means that the new function can also ensure that there is a closing bracket before performing a substitution.
Configuration files can now interpolate the contents of a file, so a
secret mounted into the container never has to be copied into an
environment variable to reach `pgdog.toml`:
```toml
[admin]
password = "${file./run/secrets/admin_password}"
```
Environment references move under an `env.` prefix at the same time:
`${env.VAR}` where it used to be `${VAR}`. Both prefixes are required,
so a plain `${VAR}` is now literal text. The two forms are otherwise
identical — `${env.VAR:-value}` and `${file.PATH:-value}` both supply a
fallback, and `$${env.VAR}` is still a literal `${env.VAR}`.
This is a breaking change to syntax introduced in the two commits right
before it and never released, so nothing in the wild depends on the
unprefixed form. The namespace is what makes a second reference kind
possible without guessing at the target from its shape, and it widens
the set of values that pass through untouched: a `${...}` that isn't one
of the two known prefixes is left alone rather than being treated as a
lookup that happened to miss.
File references are strict where environment references are lenient. An
unset variable is left in the document verbatim, because `users.toml` is
full of values that may legitimately contain a `$` and failing the load
on one would be worse than leaving it be. A `file.` reference names a
path the operator wrote down on purpose, so a file that can't be read is
an error — `Error::FileReference`, carrying the path and the underlying
`io::Error` — unless a fallback is given. A silently empty password is
the failure mode worth avoiding here.
Trailing newlines are trimmed from file contents. Secret managers and
`echo` alike conventionally leave one behind, and since expansion runs on
the document source before it is parsed, that newline would otherwise
land in the middle of a TOML string and change how the rest of the
document parses.
Implementation notes:
- `expand()` now returns `Result<Cow<'_, str>, Error>`; it still returns
`Cow::Borrowed` when the source contains no `${`, so the common case
costs no allocation. `FromToml::from_toml` propagates the error.
- A `Reference` enum carries the parsed target, so the recognition step
stays where the `}` is found and the substitution step just matches on
what was recognised.
- `is_path` is deliberately loose — non-empty, no whitespace, no `$` or
`{`. It exists to decide reference-or-literal, not to validate a path;
anything stricter would reject paths that the filesystem accepts, and
`read_to_string` reports the real answer anyway.
- Only trailing `\r` and `\n` are trimmed, not interior newlines or
trailing spaces, both of which can be part of a secret.
- Tests write real files with `tempfile` rather than mocking the read,
which keeps the newline-trimming and missing-file cases honest.
Configuration files can now reference the process environment and the contents of files on disk, so secrets and per-environment values no longer have to be baked into
pgdog.tomlandusers.toml:Two reference kinds, both requiring the braced, prefixed form:
${env.VAR}— the environment variableVARfrom the process environment.${file.PATH}— the contents of the file atPATH, with trailing newlines trimmed. A relative path resolves against the current working directory.Both accept a fallback (
${env.VAR:-value},${file.PATH:-value}), and$${env.VAR}is a literal${env.VAR}.Everything else is literal text: a bare
$VAR, an unprefixed${VAR}, a malformed or unterminated${, and anenv.reference to a variable that isn't set. That leniency is what keeps existing configs working —users.tomlis the file most likely to contain a stray$, so passwords likesup$rsecretorp$$w0rdpass through untouched instead of turning into a startup failure or a silently truncated credential.file.references are strict whereenv.references are lenient. Afile.reference names a path the operator wrote down on purpose, so a file that can't be read is an error —Error::FileReference, carrying the path and the underlyingio::Error— unless a fallback is given. A silently empty password is the failure mode worth avoiding there. Trailing newlines are trimmed because secret managers andechoalike conventionally leave one behind.Expansion runs on the document source before it is parsed, so a reference is interpolated as TOML rather than as a string.
${env.PASSWORD}in value position still needs its surrounding quotes, and a value containing"or a newline will change how the rest of the document parses. This is also what allows bareshutdown_timeout = ${env.VAR}to work, and it is documented onexpand.Implementation notes:
pgdog-config::expandmodule.expand()returnsCow::Borrowedwhen the source contains no${, so the common case costs no allocation. AReferenceenum carries the parsed target, so recognition stays where the}is found and substitution just matches on what was recognised.${emits through the${and rescans immediately after it, so a stray${in one value can't swallow a real reference later in the document.is_nameis the shell rule (letters, digits, underscores, not leading with a digit).is_pathis deliberately loose — non-empty, no whitespace, no$or{. It exists to decide reference-or-literal, not to validate a path; anything stricter would reject paths the filesystem accepts, andread_to_stringreports the real answer anyway.\rand\nare trimmed, not interior newlines or trailing spaces, both of which can be part of a secret.FromToml::from_tomlreplaces baretoml::from_strat the three sites that parse config text read from disk: both branches ofConfigAndUsers::loadandbootstrap_logger. Every othertoml::from_strin the tree parses a test literal, where expansion is unwanted, and is untouched.DeserializeOwned, so no per-type boilerplate is needed.from_toml, notfrom_str, to avoid colliding with the crate's manystd::str::FromStrimpls.Error::confignow receives the expanded text, so the line numbers it reports stay correct when a reference's value contains a newline.ConfigAndUserskeepsconfig_text/users_textas the raw, unexpanded source. Resolved secrets must not be written back to disk when the config is reloaded or backed up.tempfilerather than mocking the read, which keeps the newline-trimming and missing-file cases honest.The branch's first commit introduced the unprefixed
${VAR}form and the third moved it underenv.; that syntax was never released, so nothing in the wild depends on it. The namespace is what makes a second reference kind possible without guessing at the target from its shape, and it widens the set of values that pass through untouched: a${...}that isn't one of the two known prefixes is left alone rather than being treated as a lookup that happened to miss.Fixes #1479